-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clearer error messages for private functions with #[spirv(fragment)] attribute #161
base: main
Are you sure you want to change the base?
Clearer error messages for private functions with #[spirv(fragment)] attribute #161
Conversation
Oops, forgot to run cargo fmt before. Sorry about that, new commit now. |
Hi again! Was feeling unpleased with this solution and wanted to go back to it. // rust-gpu/crates/spirv-std/macros/src/lib.rs/spirv()
// prepend with #[rust_gpu::spirv(..)]
let attr: proc_macro2::TokenStream = attr.into();
tokens.extend(quote! { #[cfg_attr(target_arch="spirv", rust_gpu::spirv(#attr))] });
- if attr.to_string().trim() == "fragment" {
- let item_clone = item.clone();
- let input = syn::parse_macro_input!(item_clone as ItemFn);
- if !matches!(input.vis, Visibility::Public(_)) {
- panic!("The `spirv` macro can only be applied to public functions.");
- }
- }
+ if let Ok(item_fn) = syn::parse::<ItemFn>(item.clone()) {
+ if !matches!(item_fn.vis, Visibility::Public(_)) {
+ panic!("The `spirv` macro can only be applied to public functions.");
+ }
+ }
let item: proc_macro2::TokenStream = item.into();
for tt in item { This works great! However, there are some functions instead of the spirv_std source that are not public. I don't know much about these functions, so I'd love to know if that was intentional, or if we can go ahead and make them public. I've pushed a new commit with those changes to be viewed. However, if the two function changes are not wanted, we can do it another way! |
Oops, apparently I missed a lot more private functions. I can fix this in the morning if that's the way that the code owners want to go with it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR again! I'm so glad you chose us to continue your open source adventure 🍻.
For this one I think we'll want a different fix. Looking at #155, we do detect the case already somewhere, the message is just bad. The message also points out that there is a valid way to not have an entry point entirely ("Linkage"), though I am personally unfamiliar with that functionality.
So I would suggest enumerating the cases, finding the error message from #155, and see if the code at that point has enough info to differentiate the different cases and have better / more specific error messages. If not, figure out how to plumb the required information down to the point that message is displayed if possible.
Again, I am unfamiliar with this code personally so I don't know if this is still a quick fix or very involved...more exploration needed!
So! As far as I can tell, nothing close to this error message shows up anywhere in the rust-gpu repo. However, the exact same error message showed up in another repo, SPIRV-Tools#2538. They fixed is by going from In addition to this, when I change functions from public to private in the rust-gpu examples, such as error: function `main_fs` is never used
--> examples/shaders/mouse-shader/src/lib.rs:140:4
|
140 | fn main_fs(
| ^^^^^^^ which makes it seem even more to me like it isn't being checked on the rust side. |
Working off of this ticket
Unsure about this one honestly. I do think that it works, but I'm having to clone item and I'm unsure if that's wanted.
Also not sure if the language used for the error is exactly what is wanted, but I do think that this technically works and it clearer than what was seen before.
This is also currently only detecting for fragment shaders just since only that was mentioned in the original ticket. However, it'd be pretty easy to add other checks (such as for
#[spirv(vertex)]
).Any feedback is appreciated, thanks!